Add RelationalCommandCaching based on parameter value nullability #18035
Conversation
|
Implementation follows what we had in previous versions. |
...e.Relational/Query/RelationalShapedQueryCompilingExpressionVisitor.RelationalCommandCache.cs
Outdated
Show resolved
Hide resolved
|
Tests? |
|
Do you have any ideas in mind, how to test this? A way to catch regression would be our perf tests. |
|
You can add unit tests with a mock QuerySqlGenerator and assert how many times a command was created. |
...e.Relational/Query/RelationalShapedQueryCompilingExpressionVisitor.RelationalCommandCache.cs
Show resolved
Hide resolved
|
We should also consider extracting some nested classes out of the partial classes. The fact that they are partial is a smell and it's ok to have a separate type even if it's only used in one place. |
|
They are implementation detail of this visitor hence stayed inside. Is partial the issue or nesting? |
|
@smitpatel The partial means that the class is too big and should be split. And in practice you don't gain much by keeping the class nested. |
smitpatel
force-pushed
the
somethingfuntodo
branch
2 times, most recently
from
7da3faf to
7db14d5
Compare
|
@AndriySvyryd - Since this is also cache, do we need to do anything specific for cache eviction? |
|
@smitpatel Yes, use MemoryCache and set size for the entries |
smitpatel
force-pushed
the
smit/whereareyouhiding
branch
from
7360972 to
4562f7b
Compare
|
Filed #18493 for tests |
| { | ||
| private class RelationalCommandCache | ||
| { | ||
| private readonly ConcurrentDictionary<CommandCacheKey, IRelationalCommand> _commandCache |
There was a problem hiding this comment.
I thought I was missing something. 😄
Resolves #15892
I attempted to add to cosmos also but ran into #18033
Currently this takes naïve approach that if SelectExpression was modified during parameter based EV then we cannot cache it. If we work on #16375 then that would be quite accurate logic without us having to pass back the information. I filed #18034 to track.